Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

EK60, add mandatory transmit_type as a scalar #1101

Merged
merged 1 commit into from
Jul 31, 2023

Conversation

emiliom
Copy link
Collaborator

@emiliom emiliom commented Jul 30, 2023

@emiliom emiliom added the data format Anything about data format label Jul 30, 2023
@emiliom emiliom added this to the 0.8.0 milestone Jul 30, 2023
@emiliom emiliom requested a review from leewujung July 30, 2023 19:29
@codecov-commenter
Copy link

codecov-commenter commented Jul 30, 2023

Codecov Report

Merging #1101 (0f183e5) into dev (13be688) will decrease coverage by 0.48%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##              dev    #1101      +/-   ##
==========================================
- Coverage   78.12%   77.65%   -0.48%     
==========================================
  Files          65       18      -47     
  Lines        6227     2927    -3300     
==========================================
- Hits         4865     2273    -2592     
+ Misses       1362      654     -708     
Flag Coverage Δ
unittests 77.65% <ø> (-0.48%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed Coverage Δ
echopype/convert/set_groups_ek60.py 92.74% <ø> (ø)

... and 54 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Member

@leewujung leewujung left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good to add this!

Question: The "Continuous Wave" flag_meanings seems to come from the EK80 reference manual? I wonder if we should say add "(gated single-frequency sine waves)" for added explanation.

Also, for FMD, the "D" seems to be user-defined?

@emiliom
Copy link
Collaborator Author

emiliom commented Jul 31, 2023

Question: The "Continuous Wave" flag_meanings seems to come from the EK80 reference manual? I wonder if we should say add "(gated single-frequency sine waves)" for added explanation.

Right. But I also found a reference to CW in the EK60 manual, though it was indirect.

Also, today I realized that the convention uses a special enum type for transmit_type, not a string per se. Here's what the enum description says:

byte enum transmit_t {CW = 0, LFM = 1, HFM = 2}: "Type of transmit pulse. CW = continuous wave – a pulse nominally of one frequency, LFM = linear frequency modulation – a pulse which varies from transmit_frequency_start to transmit_frequency_stop in a linear manner over the nominal pulse duration (transmit_duration_nominal), HFM = hyperbolic frequency modulation – a pulse which varies from transmit_frequency_start to transmit_frequency_stop in a hyperbolic manner over the nominal pulse duration (transmit_duration_nominal)."

Also, for FMD, the "D" seems to be user-defined?

In the EK80 manual under PulseForm (p. 82), FMD is not defined. But I just noticed that "FMD" does appear in one other place (p. 87), under ADCP PulseForm, as "LFMD". There, the acronyms are spelled out, but the LFMD part is kind of jumbled:

image

It's hard to be sure, but it does look like "D" might stand for user-defined.

Tweaking the attributes is easy to do at any time, and has no real downstream impact. But I wonder if we need to revisit transmit_type one last time in light of the convention enum type .... Sigh.

@leewujung
Copy link
Member

but the LFMD part is kind of jumbled:
[...]
It's hard to be sure, but it does look like "D" might stand for user-defined.

Yes... in fact that was where I found the "user-defined" because I was just doing a full text search.

Tweaking the attributes is easy to do at any time, and has no real downstream impact. But I wonder if we need to revisit transmit_type one last time in light of the convention enum type .... Sigh.

... I thought you said enum type is not supported in netCDF (I forgot in which issue/PR)?

@emiliom emiliom self-assigned this Jul 31, 2023
@emiliom
Copy link
Collaborator Author

emiliom commented Jul 31, 2023

... I thought you said enum type is not supported in netCDF (I forgot in which issue/PR)?

The thread you're referring to is probably this one: #1097 (comment). I'll paste it here: "let's not try to bring some kind of enum type, which I don't think actually exists in the netcdf/CF/zarr world. I don't think there's a boolean type either, so I'll use np.byte."

After some digging, I can see I misspoke, partly. The enum type does exist in netcdf4 (which should've been obvious to me, given that it's used in the convention!). However, it doesn't appear to be widely implemented. As far as I can tell it's not supported by CF or xarray. I see no statements saying "enum types are not supported", but a search through their documentation doesn't say they do; CF only discusses "enumerations" in the context of the "flag" attributes -- their way of representing enums via attributes rather than via hard coding a custom data type. The xarray documentation has only one hit for the string "enum", and I think it refers to something else. Also, I don't think I've ever run into a netcdf enum type in actual use.

In summary: NetCDF4 (at least the C library -- I haven't checked the Python library) does support enum types, but to me, for all practical purposes it seems impractical, maybe even unwise, to try to use them.

@emiliom
Copy link
Collaborator Author

emiliom commented Jul 31, 2023

Since you approved this PR (thanks!), I'll merge it and we can continue the transmit_type and enum discussion here or in a new issue.

@emiliom emiliom merged commit de4fb4d into OSOceanAcoustics:dev Jul 31, 2023
3 checks passed
@emiliom emiliom deleted the ek60-transmittype branch July 31, 2023 19:59
@leewujung
Copy link
Member

In summary: NetCDF4 (at least the C library -- I haven't checked the Python library) does support enum types, but to me, for all practical purposes it seems impractical, maybe even unwise, to try to use them.

Let's not worry enum type then in echopype, which makes things easier. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
data format Anything about data format
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants